-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FormToggle: refine animation #56515
FormToggle: refine animation #56515
Conversation
Size Change: +115 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well! ✅
The only thing is I found the animation a bit quick, but since 'reduced motion' is my default setting, I might not be the best to judge on that. 😅 I'd prefer 0.3s
but will leave it up to you and anyone else who'd like to chime in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working as expected even in Windows' high contrast mode 👍
Additionally, trunk had a small dot in the center of the thumb when the control is unchecked, which is resolved with this PR. The reason is that the border width has been changed from 5px to 6px.
trunk
trunk.mp4
This PR
pr.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good; one tweak would be to consider setting the transitions at the same length of time.
$transition-duration-color: 0.2s; | ||
$transition-duration-position: 0.1s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why these shouldn't be the same 0.2 value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is with 0.2s on both:
Area.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, I kept the same transition durations as currently on trunk
while refactoring the code slightly. Happy to have all durations be the same — 0.2s
or any other value that you would recommend (Brooke suggested 0.3s
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and updated all transitions to have the same duration (0.2s
). I'll go ahead and merge this PR, but I'm happy to iterate further in a follow-up if we need to make further tweaks
228f3e0
to
aa82396
Compare
Flaky tests detected in aa82396. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7034490479
|
border: 5px solid $gray-900; // Has explicit border to act as a fill in Windows High Contrast Mode. | ||
|
||
// Transparent border acts as a fill in Windows High Contrast Mode. | ||
border: $thumb-size / 2 solid transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do this, to enable the SASS:
border: #{ $thumb-size / 2 } solid transparent;
That will make the calculation in the sass part of things, instead of the CSS part of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, I might be wrong. The other option is to do $thumb-size * 0.5
— I think it's because the slash is now also used for some grid CSS properties, so it's not good as a math operator anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! I'll spin up a PR to implement @jasmussen 's suggestion. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official recommendation is to @use "sass:math"
. So in theory, this...
border: math.div($thumb-size, 2) solid transparent;
* ToggleForm: refine animation * CHANGELOG * Use same transition duration for both color and position-based styles
What?
Improve the animation and the high-contrast mode styles for
FormToggle
component, which is also used in theToggleControl
componentWhy?
In the current version of the component, the track border and the thumb color are not animated when the toggle is flipped.
An improved animation gives a better sense of polish to the component and the UI in general.
Improvements to how the component is rendered in High Contrast mode benefit assistive technology users.
How?
By applying a few tweaks to
FormToggle
's styles:Testing Instructions
FormToggle
andToggleControl
storybook examplesFormToggle
orToggleControl
Screenshots or screencast
toggle-form.mp4